net,child_process: improve naming in internal code #14449
Conversation
All of this code is internal-only, and the changed variables/methods are not generally useful to userland code. When backporting this to release branches, it might be appropriate to add non-enumerable aliases to be 100 % sure.
nodejs-github-bot
added
child_process
|
While I'm definitely +1 on the change, there's a non-zero chance that this will break existing code. While we shouldn't be too sympathetic to users access undocumented private properties, we know they do so. The old property names should go through a proper deprecation cycle and be recast as aliases to the new names, at least until Node.js 10.0.0 |
jasnell
added
the
semver-major
Trott
dismissed
their stale review
Ooof, yeah, deprecation cycle as mentioned by @jasnell...Totally +1 to moving in this direction though, let's do it!
|
@Trott This is already getting a CITGM run. :) CI is green modulo unrelated failures, I don’t see anything concerning in the CITGM output. @jasnell I know you like deprecations but it’s already pretty unrealistic that userland code touches these methods to begin with. They are not conveying information that would generally be useful to the outside world. If you are 100 % sure that’s the best idea, I can add deprecated aliases here, but I think it’s much more likely to be unnecessary overhead (and re-introducing inacceptable language into our documentation) if we do so. |
|
We've been bitten before when making assumptions about how unlikely it is that someone is using a private API. Making this change is the right thing to do, but so is protecting existing users. We should do both. |
|
@jasnell done, PTAL |
| @@ -634,6 +634,17 @@ Type: Runtime | |||
|
|
|||
| *Note*: change was made while `async_hooks` was an experimental API. | |||
|
|
|||
| <a id="DEP00XX"></a> | |||
There was a problem hiding this comment.
As far as I see from Git history, deprecations are usually assigned a number immediately in the pull request.
There was a problem hiding this comment.
I don’t care much but I think @jasnell has been telling people multiple times to assign the number while landing :)
There was a problem hiding this comment.
yeah, it's better to assign when landing... or likely better yet to handle it like the REPLACEME metadata eventually. The reason is because if we end up with more than one deprecation PR at any given time, there's a greater chance of accidentally duplicating the codes or getting them out of order somehow.
There was a problem hiding this comment.
Ok, thanks for clarifying :)
|
Landed in 75a19fb |
All of this code is internal-only, and the changed variables/methods are not generally useful to userland code. When backporting this to release branches, it might be appropriate to add non-enumerable aliases to be 100 % sure. PR-URL: #14449 Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com> Reviewed-By: Rich Trott <rtrott@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Missed while landing 75a19fb. Ref: nodejs#14449
Remove the getters introduced in 75a19fb. Refs: nodejs#14449
Remove the getters introduced in 75a19fb. PR-URL: #17141 Refs: #14449 Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Anatoli Papirovski <apapirovski@mac.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Gireesh Punathil <gpunathi@in.ibm.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Evan Lucas <evanlucas@me.com> Reviewed-By: James M Snell <jasnell@gmail.com>
[trigger warning for use of racist language in the actual diff]
All of this code is internal-only, and the changed variables/methods are not generally useful to userland code.
When backporting this to release branches, it might be appropriate to add non-enumerable aliases to be 100 % sure.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
net, child_process